Skip to content

fix: keep artifact checksums in sync#2973

Open
matthewflint wants to merge 4 commits intoopenai:mainfrom
matthewflint:fix/sandbox-artifact-checksum-sync
Open

fix: keep artifact checksums in sync#2973
matthewflint wants to merge 4 commits intoopenai:mainfrom
matthewflint:fix/sandbox-artifact-checksum-sync

Conversation

@matthewflint
Copy link
Copy Markdown
Contributor

@matthewflint matthewflint commented Apr 20, 2026 •
edited
Loading

Summary

This PR is intentionally stacked on top of #2972.

On top of the safe LocalFile open path from that PR, this change:

  • computes artifact checksums from the same bytes written into the sandbox
  • preserves local source-read errors when write backends wrap stream failures
  • avoids deleting dest from the artifact layer based only on source stream reads
  • adds focused LocalFile / LocalDir regressions for checksum drift, wrapped read failures, and staged write failures preserving existing destinations

Testing

  • .venv/bin/python -m pytest -q tests/sandbox/test_entries.py
  • .venv/bin/ruff check src/agents/sandbox/entries/artifacts.py tests/sandbox/test_entries.py
  • .venv/bin/ruff format --check src/agents/sandbox/entries/artifacts.py tests/sandbox/test_entries.py
  • .venv/bin/python -m py_compile src/agents/sandbox/entries/artifacts.py tests/sandbox/test_entries.py
  • git diff --check

@matthewflint
fix(sandbox): Reject symlinked LocalFile sources
0a00da3 
Route LocalFile reads through the same pinned path handling used by LocalDir so symlinked source paths are rejected consistently. Add focused regression coverage for symlinked ancestor and leaf paths.
chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed Apr 20, 2026
View reviewed changes
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 344bb4728a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/agents/sandbox/entries/artifacts.py Outdated
chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed Apr 20, 2026
View reviewed changes
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 23168abe77

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/agents/sandbox/entries/artifacts.py Outdated
@matthewflint
fix(sandbox): validate LocalFile sources before hashing
75ba2ce 
Signed-off-by: matthewflint <277024436+matthewflint@users.noreply.github.com>
@seratch seratch marked this pull request as draft April 20, 2026 21:26
@seratch seratch changed the title Keep artifact checksums in sync fix: keep artifact checksums in sync Apr 20, 2026
@github-actions github-actions Bot added the bug Something isn't working label Apr 21, 2026
@matthewflint matthewflint force-pushed the fix/sandbox-artifact-checksum-sync branch from e0d869b to 9a939ce Compare April 21, 2026 16:24
@matthewflint
Copy link
Copy Markdown
Contributor Author

matthewflint commented Apr 21, 2026

Restacked this on top of #2972 and force-pushed a clean history. The PR base is still main, so it includes the #2972 changes for now; intended merge order is #2972 first, then this.

@matthewflint matthewflint force-pushed the fix/sandbox-artifact-checksum-sync branch from 9a939ce to 6407cd1 Compare April 21, 2026 16:31
@matthewflint
Copy link
Copy Markdown
Contributor Author

matthewflint commented Apr 21, 2026 •
edited
Loading

Restacked this on the updated #2972 head and force-pushed. It now inherits the Windows metadata assertion fix; intended merge order is still #2972 first, then this PR.

@matthewflint matthewflint marked this pull request as ready for review April 21, 2026 16:32
@matthewflint matthewflint force-pushed the fix/sandbox-artifact-checksum-sync branch 2 times, most recently from bb060d5 to 9f8b53c Compare April 22, 2026 07:01
seratch
seratch requested changes Apr 26, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall, this looks good to me; can you resolve the conflicts with your #2972 ?

@seratch seratch added this to the 0.14.x milestone Apr 26, 2026
@matthewflint matthewflint force-pushed the fix/sandbox-artifact-checksum-sync branch from 9f8b53c to 125415c Compare April 26, 2026 14:57
@matthewflint
Copy link
Copy Markdown
Contributor Author

matthewflint commented Apr 26, 2026

Thanks, #2972 is merged now. I rebased this on the latest main and resolved the conflict.

I kept the streaming checksum path, preserved the LocalFile source validation from #2972, and added coverage for checksum consistency, wrapped read errors, fallback validation, and partial-write cleanup.

chatgpt-codex-connector[bot]
chatgpt-codex-connector Bot reviewed Apr 26, 2026
View reviewed changes
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 125415c7ca

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/agents/sandbox/entries/artifacts.py Outdated
@matthewflint matthewflint marked this pull request as draft April 26, 2026 15:06
@matthewflint matthewflint force-pushed the fix/sandbox-artifact-checksum-sync branch from 125415c to b79ac96 Compare April 28, 2026 15:05
@matthewflint
Copy link
Copy Markdown
Contributor Author

matthewflint commented Apr 28, 2026

Addressed. The artifact layer no longer deletes dest based only on source stream reads. LocalFile and LocalDir now share the same hashed streaming write helper, so WorkspaceArchiveWriteError can still unwrap nested LocalFileReadError correctly. Added regressions for staged write failures preserving an existing destination while keeping checksums based on the bytes streamed to the sandbox.

@matthewflint matthewflint marked this pull request as ready for review April 28, 2026 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

@seratch seratch seratch requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

bug Something isn't working feature:sandboxes

Projects

None yet

Milestone

0.14.x

Development

Successfully merging this pull request may close these issues.

2 participants

@matthewflint @seratch